ACM-14557-Implement inferred ranges for cluster list advanced search #3913
Conversation
|
/hold Seeking feedback |
Randy424
force-pushed
the
ACM-14557-advanced-search-inferred-ranges
branch
from
962dc38
to
fde46c0
Compare
| const incompleteVersionData = { | ||
| versionOneIncomplete: '1.', | ||
| versionTwoIncomplete: '1.5', | ||
| versionThreeIncomplete: '2', | ||
| versionFourIncomplete: '2.5', | ||
| } | ||
|
|
There was a problem hiding this comment.
If we're testing 1. should we also test 2.5.?
frontend/src/lib/search-utils.ts
Outdated
| ? semver.inc(coercedInputVersion, 'major') | ||
| : semver.inc(coercedInputVersion, 'minor') | ||
|
|
||
| const equalsValueRange = `${coercedInputVersion} - ${topRange}` |
There was a problem hiding this comment.
I think this would include topRange, but it should be excluded.
I think we can just use "X" or "X.Y" as a range. See X-Ranges 1.2.x 1.X 1.2.* *: "A partial version range is treated as an X-Range, so the special character is in fact optional."
semver.satisfies("5.0.0", "4.0.0 - 5.0.0")
true
semver.satisfies("5.0.0", "4")
false
semver.satisfies("4.14.50", "4.14")
true
semver.satisfies("4.17.3", "4")
true
frontend/src/lib/search-utils.ts
Outdated
| case SearchOperator.GreaterThan: | ||
| return semver.satisfies(coercedVersionToCompare, `>${coercedInputVersion}`) | ||
| case SearchOperator.GreaterThanOrEqualTo: | ||
| return semver.satisfies(coercedVersionToCompare, `>=${coercedInputVersion}`) | ||
| case SearchOperator.LessThan: | ||
| return semver.satisfies(coercedVersionToCompare, `<${coercedInputVersion}`) | ||
| case SearchOperator.LessThanOrEqualTo: | ||
| return semver.satisfies(coercedVersionToCompare, `<=${coercedInputVersion}`) |
There was a problem hiding this comment.
Is there a reason we are changing these ones to use satisfies if they are not using a range?
Also, I think we should refactor this code to avoid repeating the switch statement.
There was a problem hiding this comment.
If I'm understanding you correctly, I believe using satisfies is still necessary in this situation, as this language (e.g. >${coercedInputVersion}) is actually part of the range grammar, and would not work with something like semver.gt.
Otherwise, big copy-that on the refactor.
There was a problem hiding this comment.
But coercedInputVersion itself is not a range. It's only a range because we added the operator before it. I think it's a valid question of whether we want to use a range in this case or not though.
To test this, in your file I added window.semver = semver so I could easily call semver functions in the browser console.
Suppose a user has entered "version>4.12"
semver.gt("4.12.1", "4.12.0") is true
semver.satisfies("4.12.1" ">4.12") is false
To me the latter makes sense because the user did not specify the patch version, so they are looking for 4.13 or higher.
I opened a PR against your PR with simplified implementation.
Randy424#20
There was a problem hiding this comment.
Also, this matches how npm does version ranges. See https://semver.npmjs.com/
Enter "lodash" and ">1" and you'll see the lowest version is 2.0.0.
frontend/src/lib/search-utils.ts
Outdated
| inputVersion: string, | ||
| operator: SearchOperator | ||
| ) => { | ||
| const coerceVersion = (version: string) => semver.valid(semver.coerce(version)) ?? version |
There was a problem hiding this comment.
Does using semver.valid accomplish anything here? I think semver.coerce returns either a valid SemVer or null. Perhaps we could just do const coercedInputVersion = semver.coerce(inputVersion) and then just check for a value in the later conditions, rather than checking the result of semver.coerce again.
Signed-off-by: Randy Bruno Piverger <rbrunopi@redhat.com>
Randy424
force-pushed
the
ACM-14557-advanced-search-inferred-ranges
branch
from
fde46c0
to
c316c0a
Compare
| it('can infer on incomplete semver equals', () => { | ||
| expect(handleSemverOperatorComparison(versionOne, versionOneIncomplete, SearchOperator.Equals)).toBeTruthy() | ||
| }) | ||
| it('can infer on incomplete semver greater than', () => { |
There was a problem hiding this comment.
This test fails with my proposed PR, which makes sense if we consider >1 to mean 2 or higher. (1.5.0 is the first arg here)
By the way, using constants for the test values here just makes it hard to understand and debug the tests. I would just repeat the numbers in the tests.
Signed-off-by: Kevin Cormier <kcormier@redhat.com>
Signed-off-by: Randy Bruno Piverger <rbrunopi@redhat.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KevinFCormier, Randy424 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
|
Regarding: ACM-14557
When input in the Advanced Search Field is incomplete the intuitive behavior (what is being implemented here) is that an assumed range of values could appear in the search results.
For example: "Distribution version = 4.", Here the table should display all values greater than or equal to 4.0.0 but less than 5.0.0
Before:
After:
